feat(eth): protos, go-ethereum, eth types, and evm module types #1837
Conversation
WalkthroughThis extensive update brings Ethereum Virtual Machine (EVM) compatibility to the project, introducing new protobuf files for Ethereum transactions, logs, and queries. It also includes enhancements in Ethereum cryptography and type handling, as well as integration of Ethereum's JSON-RPC specifications to support Solidity-based decentralized applications (dApps). Changes
Possibly related issues
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (11)
eth/types/account.pb.gois excluded by!**/*.pb.go,!**/*.pb.goeth/types/dynamic_fee.pb.gois excluded by!**/*.pb.go,!**/*.pb.goeth/types/indexer.pb.gois excluded by!**/*.pb.go,!**/*.pb.goeth/types/web3.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/evm/types/events.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/evm/types/evm.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/evm/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/evm/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/evm/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.gox/evm/types/tx.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/evm/types/tx.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
Files selected for processing (9)
- proto/eth/evm/v1/events.proto (1 hunks)
- proto/eth/evm/v1/evm.proto (1 hunks)
- proto/eth/evm/v1/genesis.proto (1 hunks)
- proto/eth/evm/v1/query.proto (1 hunks)
- proto/eth/evm/v1/tx.proto (1 hunks)
- proto/eth/types/v1/account.proto (1 hunks)
- proto/eth/types/v1/dynamic_fee.proto (1 hunks)
- proto/eth/types/v1/indexer.proto (1 hunks)
- proto/eth/types/v1/web3.proto (1 hunks)
Additional comments not posted (13)
proto/eth/types/v1/dynamic_fee.proto (1)
1-13: LGTM! The definition ofExtensionOptionDynamicFeeTxcorrectly specifies the max priority price for Cosmos transactions, aligning with the EIP-1559 specification. The use ofcosmossdk.io/math.Intformax_priority_priceis appropriate for handling large numbers.proto/eth/types/v1/account.proto (1)
1-26: LGTM! TheEthAccountmessage is correctly defined to ensure compatibility with Ethereum accounts and the Cosmos SDK. The use ofgogoprotooptions and the embedding ofBaseAccountare appropriately applied.proto/eth/types/v1/web3.proto (1)
1-26: LGTM! TheExtensionOptionsWeb3Txmessage is correctly defined to support Web3 transactions, including typed chain ID and fee delegation, aligning with EIP712 specifications.proto/eth/types/v1/indexer.proto (1)
1-31: LGTM! TheTxResultmessage is well-structured to capture essential details of Ethereum transactions for indexing purposes. The fields chosen are appropriate for tracking transaction processing and gas usage.proto/eth/evm/v1/genesis.proto (1)
1-28: LGTM! TheGenesisStateandGenesisAccountmessages are correctly defined to represent the initial state for the EVM module, including genesis accounts with their code and storage.proto/eth/evm/v1/events.proto (1)
1-45: LGTM! The event messages introduced in this file are well-structured to capture essential details of Ethereum transactions, logs, and block blooms, ensuring detailed logging and event representation for the EVM module.proto/eth/evm/v1/tx.proto (1)
1-178: LGTM! The messages and service defined in this file correctly represent various Ethereum transaction types and provide a structured way to submit transactions and update module parameters for the EVM module.proto/eth/evm/v1/evm.proto (1)
1-222: LGTM! The messages defined in this file correctly represent various configurations, states, and logs for the EVM module, ensuring a robust and configurable EVM implementation. TheTraceConfigmessage, in particular, provides comprehensive tracing options, which are crucial for debugging and analysis.proto/eth/evm/v1/query.proto (5)
14-303: Ensure all newly introduced gRPC service methods and message types are thoroughly documented.
Adding comments to explain the purpose and usage of each service method and message type enhances code readability and maintainability. It's especially important for public APIs, as it aids developers in understanding how to interact with the services.
18-18: Verify the REST endpoint paths for consistency and adherence to RESTful design principles.
The REST endpoint paths should be consistent and follow RESTful design principles. For example, using plural nouns (accountsinstead ofaccount) and ensuring a logical hierarchy in the paths can improve the API's intuitiveness.
53-66: Consider adding POST methods forEthCall,EstimateGas, andTraceTxservices.
TheEthCall,EstimateGas, andTraceTxservices currently use GET methods, which may not be suitable for operations that involve complex request bodies. Consider changing these to POST methods to support more complex and potentially large request payloads.
219-228: Review the use ofbytestype for fields that represent addresses or other hex-encoded strings.
For fields likeproposer_addressinEthCallRequest, consider using a string type if these addresses are expected to be hex-encoded. This can improve clarity and reduce the need for conversions in client code.
302-302: Ensure the custom typecosmossdk.io/math.Intforbase_feeis correctly implemented and tested.
The use of a custom type forbase_feeinQueryBaseFeeResponsesuggests specialized handling or functionality. Verify that this custom type is correctly implemented, especially regarding serialization and deserialization, and thoroughly tested to handle various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (1)
CHANGELOG.md (1)
63-63: The changelog entry is well-documented and follows the standard format, providing clear information about the new feature related to Ethereum in the Nibiru EVM module.
* feat(eth): unit tests for types, crypto, encoding
## Impl `go-ethereum`
Adds `go-ethereum` fork with an `Interpreter` interface for running smart
contracts in integration tests.
```go
type Interpreter interface {
// Run loops and evaluates the contract's code with the given input data and returns
// the return byte-slice and an error if one occurred.
Run(contract *Contract, input []byte, static bool) ([]byte, error)
}
```
An `Interpreter` is used to run Ethereum based contracts and will utilize the
passed environment to query external sources for state information.
The Interpreter will run the byte code VM based on the passed
configuration.
Changes from go-ethereum v1.11:
* Set `callcode` to use `readOnly` mode for precompiled calls.
* Remove `IsStateful` function from the `PrecompiledContract` interface, as this remains unused.
* Support stateful precompiled contracts.
* Add `Address` function to `PrecompiledContract` interface.
* Implement custom active precompiles for the EVM.
* Define `Interpreter` interface for the EVM.
* Move the `JumpTable` defaults to a separate function.
* Refactor `Stack` implementation
Unique-Divine
changed the title
There was a problem hiding this comment.
Actionable comments posted: 16
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (15)
eth/crypto/ethsecp256k1/keys.pb.gois excluded by!**/*.pb.gogeth/accounts/usbwallet/trezor/messages-common.pb.gois excluded by!**/*.pb.gogeth/accounts/usbwallet/trezor/messages-ethereum.pb.gois excluded by!**/*.pb.gogeth/accounts/usbwallet/trezor/messages-management.pb.gois excluded by!**/*.pb.gogeth/accounts/usbwallet/trezor/messages.pb.gois excluded by!**/*.pb.gogeth/cmd/clef/docs/clef_architecture_pt1.pngis excluded by!**/*.pnggeth/cmd/clef/docs/clef_architecture_pt2.pngis excluded by!**/*.pnggeth/cmd/clef/docs/clef_architecture_pt3.pngis excluded by!**/*.pnggeth/cmd/clef/docs/clef_architecture_pt4.pngis excluded by!**/*.pnggeth/cmd/clef/docs/qubes/clef_qubes_http.pngis excluded by!**/*.pnggeth/cmd/clef/docs/qubes/clef_qubes_qrexec.pngis excluded by!**/*.pnggeth/cmd/clef/docs/qubes/qrexec-example.pngis excluded by!**/*.pnggeth/cmd/clef/docs/qubes/qubes_newaccount-1.pngis excluded by!**/*.pnggeth/cmd/clef/docs/qubes/qubes_newaccount-2.pngis excluded by!**/*.pnggeth/cmd/clef/sign_flow.pngis excluded by!**/*.png
Files selected for processing (56)
- CHANGELOG.md (1 hunks)
- cmd/ethclient/const.go (1 hunks)
- eth/crypto/codec/amino.go (1 hunks)
- eth/crypto/codec/codec.go (1 hunks)
- eth/crypto/ethsecp256k1/benchmark_test.go (1 hunks)
- eth/crypto/ethsecp256k1/ethsecp256k1.go (1 hunks)
- eth/crypto/ethsecp256k1/ethsecp256k1_test.go (1 hunks)
- eth/crypto/hd/algorithm.go (1 hunks)
- eth/crypto/hd/algorithm_test.go (1 hunks)
- eth/crypto/hd/benchmark_test.go (1 hunks)
- eth/crypto/hd/utils_test.go (1 hunks)
- eth/crypto/keyring/options.go (1 hunks)
- eth/crypto/secp256r1/verify.go (1 hunks)
- eth/encoding/codec/codec.go (1 hunks)
- eth/encoding/config.go (1 hunks)
- eth/encoding/config_test.go (1 hunks)
- eth/ethereum/eip712/domain.go (1 hunks)
- eth/ethereum/eip712/eip712.go (1 hunks)
- eth/ethereum/eip712/eip712_fuzzer_test.go (1 hunks)
- eth/ethereum/eip712/eip712_legacy.go (1 hunks)
- eth/ethereum/eip712/eip712_test.go (1 hunks)
- eth/ethereum/eip712/encoding.go (1 hunks)
- eth/ethereum/eip712/encoding_legacy.go (1 hunks)
- eth/ethereum/eip712/message.go (1 hunks)
- eth/ethereum/eip712/preprocess.go (1 hunks)
- eth/ethereum/eip712/preprocess_test.go (1 hunks)
- eth/ethereum/eip712/types.go (1 hunks)
- eth/types/assert.go (1 hunks)
- eth/types/chain_id.go (1 hunks)
- eth/types/chain_id_test.go (1 hunks)
- eth/types/codec.go (1 hunks)
- eth/types/errors.go (1 hunks)
- eth/types/hdpath.go (1 hunks)
- eth/types/safe_math.go (1 hunks)
- geth/.dockerignore (1 hunks)
- geth/.gitattributes (1 hunks)
- geth/.github/CODEOWNERS (1 hunks)
- geth/.github/CONTRIBUTING.md (1 hunks)
- geth/.github/ISSUE_TEMPLATE/bug.md (1 hunks)
- geth/.github/ISSUE_TEMPLATE/feature.md (1 hunks)
- geth/.github/ISSUE_TEMPLATE/question.md (1 hunks)
- geth/.github/no-response.yml (1 hunks)
- geth/.github/stale.yml (1 hunks)
- geth/.github/workflows/build.yml (1 hunks)
- geth/.github/workflows/dependencies.yml (1 hunks)
- geth/.github/workflows/lint.yml (1 hunks)
- geth/.github/workflows/markdown-links.yml (1 hunks)
- geth/.github/workflows/security.yml (1 hunks)
- geth/.github/workflows/super-linter.yml (1 hunks)
- geth/.github/workflows/test.yml (1 hunks)
- geth/.gitignore (1 hunks)
- geth/.gitmodules (1 hunks)
- geth/.golangci.yml (1 hunks)
- geth/.mailmap (1 hunks)
- geth/.markdownlint.yml (1 hunks)
- geth/.markdownlintignore (1 hunks)
Files not processed due to max files limit (39)
- geth/.protolint.yml
- geth/.travis.yml
- geth/.yamllint
- geth/AUTHORS
- geth/COPYING
- geth/COPYING.LESSER
- geth/Dockerfile
- geth/Dockerfile.alltools
- geth/Makefile
- geth/README.md
- geth/SECURITY.md
- geth/accounts/abi/abi.go
- geth/accounts/abi/abi_test.go
- geth/accounts/abi/argument.go
- geth/accounts/abi/bind/auth.go
- geth/accounts/abi/bind/backend.go
- geth/accounts/abi/bind/backends/simulated.go
- geth/accounts/abi/bind/backends/simulated_test.go
- geth/accounts/abi/bind/base.go
- geth/accounts/abi/bind/base_test.go
- geth/accounts/abi/bind/bind.go
- geth/accounts/abi/bind/template.go
- geth/accounts/abi/bind/util.go
- geth/accounts/abi/bind/util_test.go
- geth/accounts/abi/doc.go
- geth/accounts/abi/error.go
- geth/accounts/abi/error_handling.go
- geth/accounts/abi/event.go
- geth/accounts/abi/event_test.go
- geth/accounts/abi/method.go
- geth/accounts/abi/method_test.go
- geth/accounts/abi/pack.go
- geth/accounts/abi/pack_test.go
- geth/accounts/abi/packing_test.go
- geth/accounts/abi/reflect.go
- geth/accounts/abi/reflect_test.go
- geth/accounts/abi/selector_parser.go
- geth/accounts/abi/selector_parser_test.go
- geth/accounts/abi/topics.go
Files skipped from review due to trivial changes (7)
- cmd/ethclient/const.go
- geth/.gitattributes
- geth/.github/ISSUE_TEMPLATE/feature.md
- geth/.github/ISSUE_TEMPLATE/question.md
- geth/.gitignore
- geth/.mailmap
- geth/.markdownlintignore
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Additional Context Used
GitHub Check Runs (1)
lint failure (10)
eth/crypto/ethsecp256k1/ethsecp256k1.go: [failure] 12-12:
File is notgoimports-ed with -local github.com/NibiruChain/nibiru (goimports)
eth/crypto/hd/benchmark_test.go: [failure] 6-6:
File is notgoimports-ed with -local github.com/NibiruChain/nibiru (goimports)
eth/ethereum/eip712/encoding.go: [failure] 14-14:
File is notgoimports-ed with -local github.com/NibiruChain/nibiru (goimports)
eth/ethereum/eip712/encoding_legacy.go: [failure] 14-14:
File is notgoimports-ed with -local github.com/NibiruChain/nibiru (goimports)
eth/ethereum/eip712/preprocess.go: [failure] 7-7:
File is notgoimports-ed with -local github.com/NibiruChain/nibiru (goimports)
Additional comments not posted (38)
geth/.dockerignore (1)
1-5: LGTM! These changes effectively exclude unnecessary files from the Docker build context, improving build performance and reducing the Docker image size.geth/.gitmodules (1)
1-8: LGTM! The configuration of the submodules for tests and benchmarks is correct, and the use of shallow cloning is a good practice for optimizing clone operations.eth/types/errors.go (1)
1-17: LGTM! The mechanism for registering module-specific errors is correctly implemented. Consider adding more module-specific errors as needed to cover the full scope of Ethereum functionalities being integrated.geth/.markdownlint.yml (1)
1-21: LGTM! The markdownlint configuration is appropriately customized to fit the project's documentation style and requirements.geth/.github/ISSUE_TEMPLATE/bug.md (1)
1-30: LGTM! The bug report template is well-structured and comprehensive. Consider using consistent spelling (either American or British English) throughout the project documentation to maintain consistency.eth/crypto/codec/codec.go (1)
1-15: LGTM! The registration of Ethereum-specific cryptographic key types with the Cosmos SDK's interface registry is correctly implemented, enabling Ethereum compatibility.eth/ethereum/eip712/domain.go (1)
1-20: LGTM! The creation of the EIP-712 domain is correctly implemented. Consider making the domain parameters configurable in the future to support multiple environments or chains.geth/.github/no-response.yml (1)
1-11: LGTM! The configuration of the GitHub no-response bot is correctly set up, helping to maintain the cleanliness of the issue tracker by automatically closing inactive issues.geth/.github/stale.yml (1)
1-17: LGTM! The stale bot configuration follows common practices and is correctly set up.geth/.github/workflows/markdown-links.yml (1)
1-29: LGTM! The workflow for checking Markdown links is correctly set up and follows best practices for GitHub Actions.geth/.github/workflows/dependencies.yml (1)
1-28: LGTM! The workflow for dependency review and vulnerability checks is correctly set up and follows best practices for GitHub Actions.eth/types/codec.go (1)
1-26: LGTM! The interface registrations for Ethereum-specific types with the Cosmos SDK's codec are correctly implemented.geth/.github/workflows/build.yml (1)
1-33: LGTM! The build workflow is correctly set up and follows best practices for GitHub Actions.eth/crypto/codec/amino.go (1)
1-27: LGTM! The registration of Ethereum-specific cryptographic types with the Amino codec is correctly implemented.geth/.github/workflows/test.yml (2)
13-15: Ensure that using themasterbranch for therokroskar/workflow-run-cleanup-actionis intentional. It's generally safer to use a specific version or tag to avoid unexpected changes.
22-35: The test job runs conditionally based on the presence of changes in Go files,go.mod, orgo.sum. Verify that this condition aligns with the project's testing strategy, especially for changes that might not directly modify these files but still require testing.eth/encoding/codec/codec.go (2)
14-19: LGTM! The functionRegisterLegacyAminoCodeccorrectly registers codecs for various types and crypto functionalities. It follows best practices for codec registration in the Cosmos SDK.
21-26: LGTM! The functionRegisterInterfacesproperly registers interfaces for Ethereum-related types and crypto functionalities, ensuring compatibility with the Cosmos SDK's interface registry.eth/types/hdpath.go (1)
20-33: The functionNewHDPathIteratoris well-implemented, providing a flexible way to create HD path iterators. Ensure that error handling is performed wherever this function is called, as it can return an error if the base path is invalid.geth/.github/workflows/security.yml (1)
27-31: Ensure that using themasterbranch for thecosmos/gosecaction is intentional. Similar to other GitHub Actions, it's generally safer to use a specific version or tag to avoid unexpected changes.eth/types/safe_math.go (2)
15-21: The functionSafeNewIntFromBigIntcorrectly checks for the 256-bit boundary before creating a newsdkmath.Int. This is a good practice for preventing integer overflow errors.
28-35: The functionSafeInt64properly checks for overflow when casting auint64to anint64. This is crucial for ensuring data integrity and preventing potential overflow errors.eth/encoding/config_test.go (1)
17-41: The testTestTxEncodingis well-structured and covers the essential aspects of Ethereum transaction encoding. Ensure that additional test cases are added to cover edge cases and potential error conditions for a more comprehensive test coverage.geth/.github/CODEOWNERS (1)
4-24: Verify that the CODEOWNERS file is up-to-date with the current project structure and team responsibilities. Ensure that all critical paths have designated code owners to streamline the review process and maintain code quality.eth/encoding/config.go (1)
14-31: LGTM! TheMakeConfigfunction is correctly implemented and follows best practices for creating an encoding configuration.geth/.github/workflows/super-linter.yml (1)
1-38: LGTM! The GitHub Actions workflow for linting is correctly set up and follows best practices.eth/types/assert.go (1)
12-43: LGTM! The utility functions for Ethereum address and hash validations are correctly implemented and follow best practices.eth/types/chain_id.go (1)
13-55: LGTM! The regular expression and functions related to Ethereum chain ID validation and parsing are correctly implemented and follow best practices.eth/crypto/keyring/options.go (1)
13-41: LGTM! The keyring options, including supported algorithms and the configuration for the Ethereum Secp256k1 curve, are correctly implemented and follow best practices.eth/crypto/secp256r1/verify.go (1)
25-51: LGTM! The signature verification functionality using the Secp256r1 curve is correctly implemented and follows best practices.eth/types/chain_id_test.go (1)
11-86: LGTM! The test cases for chain ID parsing and validation are comprehensive and correctly implemented.eth/ethereum/eip712/eip712_legacy.go (2)
8-8: Consider removing the comment// #nosec G702 for sensitive importor provide a more detailed explanation.
While it's clear that this is meant to suppress a security linting rule, it's not immediately clear why this suppression is necessary or safe in this context. Providing more context or justification for security-related suppressions can help maintainers and reviewers understand the security posture of the code.
142-161: The functionwalkFieldsuses recursion to handle pointer and interface types, but it lacks documentation explaining its purpose and how it works. Given the complexity of reflection and the operations performed here, adding a comment explaining the rationale behind this function would improve code readability and maintainability.eth/ethereum/eip712/types.go (1)
34-87: The functioncreateEIP712Typesiterates over message payloads to create EIP-712 types. Consider adding error handling for themessagePayload.numPayloadMsgsto ensure it does not lead to an out-of-bounds error in case of malformed input.Additionally, the function could benefit from more detailed comments explaining the purpose of each step, especially since working with EIP-712 types involves complex data structures.
eth/ethereum/eip712/eip712_test.go (4)
116-388: The test functionTestEIP712is comprehensive, covering various scenarios for EIP-712 signature verification. However, it could benefit from the following improvements:
- Consider breaking down this large test function into smaller, more focused test functions. This would improve readability and make it easier to identify which specific scenarios are being tested.
- Add more detailed comments explaining the purpose and expected outcome of each test case. This would help future maintainers understand the intent behind each test.
390-424: The functionverifyEIP712SignatureVerificationperforms signature verification for EIP-712 messages. It's well-implemented, but consider adding comments to explain the rationale behind each verification step, especially the sanity check with random bytes. This would enhance the readability and maintainability of the test code.
426-492: The functionverifySignDocFlatteningand related functions (verifyPayloadAgainstFlattened,verifyPayloadMapAgainstFlattenedMap) test the flattening algorithm for EIP-712 messages. These functions are critical for ensuring the correctness of the flattening logic. Consider the following improvements:
- Add more detailed comments explaining the purpose and logic of each function, especially given the complexity of the flattening algorithm.
- Consider adding more edge case tests to ensure the flattening algorithm behaves correctly in all scenarios.
494-511: The functionverifyBasicTypedDatatests the basic functionality of TypedData generation. It's important to ensure that the TypedData generated matches the expected structure. Consider adding assertions to verify other aspects of thetypedDataobject, such as theTypesandDomainfields, to ensure comprehensive coverage of the TypedData generation logic.
| func BenchmarkPubKey_VerifySignature(b *testing.B) { | ||
| privKey, err := GenerateKey() | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| pubKey := privKey.PubKey() | ||
|
|
||
| b.ResetTimer() | ||
| b.ReportAllocs() | ||
| for i := 0; i < b.N; i++ { | ||
| msg := []byte(fmt.Sprintf("%10d", i)) | ||
| sig, err := privKey.Sign(msg) | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| pubKey.VerifySignature(msg, sig) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider pre-generating the messages outside of the benchmark loop to reduce overhead not directly related to the signature verification process being benchmarked.
+ messages := make([][]byte, b.N)
+ for i := range messages {
+ messages[i] = []byte(fmt.Sprintf("%10d", i))
+ }
- msg := []byte(fmt.Sprintf("%10d", i))
+ msg := messages[i]Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func BenchmarkPubKey_VerifySignature(b *testing.B) { | |
| privKey, err := GenerateKey() | |
| if err != nil { | |
| b.Fatal(err) | |
| } | |
| pubKey := privKey.PubKey() | |
| b.ResetTimer() | |
| b.ReportAllocs() | |
| for i := 0; i < b.N; i++ { | |
| msg := []byte(fmt.Sprintf("%10d", i)) | |
| sig, err := privKey.Sign(msg) | |
| if err != nil { | |
| b.Fatal(err) | |
| } | |
| pubKey.VerifySignature(msg, sig) | |
| } | |
| } | |
| func BenchmarkPubKey_VerifySignature(b *testing.B) { | |
| privKey, err := GenerateKey() | |
| if err != nil { | |
| b.Fatal(err) | |
| } | |
| pubKey := privKey.PubKey() | |
| messages := make([][]byte, b.N) | |
| for i := range messages { | |
| messages[i] = []byte(fmt.Sprintf("%10d", i)) | |
| } | |
| b.ResetTimer() | |
| b.ReportAllocs() | |
| for i := 0; i < b.N; i++ { | |
| msg := messages[i] | |
| sig, err := privKey.Sign(msg) | |
| if err != nil { | |
| b.Fatal(err) | |
| } | |
| pubKey.VerifySignature(msg, sig) | |
| } | |
| } |
| run: | ||
| timeout: 20m |
There was a problem hiding this comment.
Consider increasing the timeout for golangci-lint to accommodate larger codebases or more complex analyses. A timeout of 20m might be too short for comprehensive linting in some scenarios.
- timeout: 20m
+ timeout: 30mCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| run: | |
| timeout: 20m | |
| run: | |
| timeout: 30m |
| // recursivelyAddTypesToRoot walks all types in the given map | ||
| // and recursively adds sub-maps as new types when necessary. | ||
| // It adds all type definitions to typeMap, then returns a key | ||
| // to the json object's type definition within the map. | ||
| func recursivelyAddTypesToRoot( | ||
| typeMap apitypes.Types, | ||
| rootType string, | ||
| prefix string, | ||
| payload gjson.Result, | ||
| ) (string, error) { | ||
| typesToAdd := []apitypes.Type{} | ||
|
|
||
| // Must sort the JSON keys for deterministic type generation. | ||
| sortedFieldNames, err := sortedJSONKeys(payload) | ||
| if err != nil { | ||
| return "", errorsmod.Wrap(err, "unable to sort object keys") | ||
| } | ||
|
|
||
| typeDef := typeDefForPrefix(prefix, rootType) | ||
|
|
||
| for _, fieldName := range sortedFieldNames { | ||
| field := payload.Get(fieldName) | ||
| if !field.Exists() { | ||
| continue | ||
| } | ||
|
|
||
| // Handle array type by unwrapping the first element. | ||
| // Note that arrays with multiple types are not supported | ||
| // using EIP-712, so we can ignore that case. | ||
| isCollection := false | ||
| if field.IsArray() { | ||
| fieldAsArray := field.Array() | ||
|
|
||
| if len(fieldAsArray) == 0 { | ||
| // Arbitrarily add string[] type to handle empty arrays, | ||
| // since we cannot access the underlying object. | ||
| emptyArrayType := "string[]" | ||
| typesToAdd = appendedTypesList(typesToAdd, fieldName, emptyArrayType) | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| field = fieldAsArray[0] | ||
| isCollection = true | ||
| } | ||
|
|
||
| ethType := getEthTypeForJSON(field) | ||
|
|
||
| // Handle JSON primitive types by adding the corresponding | ||
| // EIP-712 type to the types schema. | ||
| if ethType != "" { | ||
| if isCollection { | ||
| ethType += "[]" | ||
| } | ||
| typesToAdd = appendedTypesList(typesToAdd, fieldName, ethType) | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| // Handle object types recursively. Note that nested array types are not supported | ||
| // in EIP-712, so we can exclude that case. | ||
| if field.IsObject() { | ||
| fieldPrefix := prefixForSubField(prefix, fieldName) | ||
|
|
||
| fieldTypeDef, err := recursivelyAddTypesToRoot(typeMap, rootType, fieldPrefix, field) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| fieldTypeDef = sanitizeTypedef(fieldTypeDef) | ||
| if isCollection { | ||
| fieldTypeDef += "[]" | ||
| } | ||
|
|
||
| typesToAdd = appendedTypesList(typesToAdd, fieldName, fieldTypeDef) | ||
|
|
||
| continue | ||
| } | ||
| } | ||
|
|
||
| return addTypesToRoot(typeMap, typeDef, typesToAdd) |
There was a problem hiding this comment.
The function recursivelyAddTypesToRoot is complex and performs multiple operations, including handling of arrays, slices, and recursive type addition. Consider the following improvements:
- Break down this function into smaller, more focused functions to improve readability and maintainability.
- Add comments to explain the logic, especially for complex sections like handling of arrays and slices, and the recursive nature of the function.
Refactoring suggestion:
- // Current implementation
+ // Suggested refactoring into smaller functions with clear responsibilitiesCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // recursivelyAddTypesToRoot walks all types in the given map | |
| // and recursively adds sub-maps as new types when necessary. | |
| // It adds all type definitions to typeMap, then returns a key | |
| // to the json object's type definition within the map. | |
| func recursivelyAddTypesToRoot( | |
| typeMap apitypes.Types, | |
| rootType string, | |
| prefix string, | |
| payload gjson.Result, | |
| ) (string, error) { | |
| typesToAdd := []apitypes.Type{} | |
| // Must sort the JSON keys for deterministic type generation. | |
| sortedFieldNames, err := sortedJSONKeys(payload) | |
| if err != nil { | |
| return "", errorsmod.Wrap(err, "unable to sort object keys") | |
| } | |
| typeDef := typeDefForPrefix(prefix, rootType) | |
| for _, fieldName := range sortedFieldNames { | |
| field := payload.Get(fieldName) | |
| if !field.Exists() { | |
| continue | |
| } | |
| // Handle array type by unwrapping the first element. | |
| // Note that arrays with multiple types are not supported | |
| // using EIP-712, so we can ignore that case. | |
| isCollection := false | |
| if field.IsArray() { | |
| fieldAsArray := field.Array() | |
| if len(fieldAsArray) == 0 { | |
| // Arbitrarily add string[] type to handle empty arrays, | |
| // since we cannot access the underlying object. | |
| emptyArrayType := "string[]" | |
| typesToAdd = appendedTypesList(typesToAdd, fieldName, emptyArrayType) | |
| continue | |
| } | |
| field = fieldAsArray[0] | |
| isCollection = true | |
| } | |
| ethType := getEthTypeForJSON(field) | |
| // Handle JSON primitive types by adding the corresponding | |
| // EIP-712 type to the types schema. | |
| if ethType != "" { | |
| if isCollection { | |
| ethType += "[]" | |
| } | |
| typesToAdd = appendedTypesList(typesToAdd, fieldName, ethType) | |
| continue | |
| } | |
| // Handle object types recursively. Note that nested array types are not supported | |
| // in EIP-712, so we can exclude that case. | |
| if field.IsObject() { | |
| fieldPrefix := prefixForSubField(prefix, fieldName) | |
| fieldTypeDef, err := recursivelyAddTypesToRoot(typeMap, rootType, fieldPrefix, field) | |
| if err != nil { | |
| return "", err | |
| } | |
| fieldTypeDef = sanitizeTypedef(fieldTypeDef) | |
| if isCollection { | |
| fieldTypeDef += "[]" | |
| } | |
| typesToAdd = appendedTypesList(typesToAdd, fieldName, fieldTypeDef) | |
| continue | |
| } | |
| } | |
| return addTypesToRoot(typeMap, typeDef, typesToAdd) |
| messagePayload, err := createEIP712MessagePayload(data) | ||
| message := messagePayload.message | ||
| if err != nil { | ||
| return apitypes.TypedData{}, err | ||
| } |
There was a problem hiding this comment.
Consider handling the error immediately after createEIP712MessagePayload to avoid accessing messagePayload.message when err is not nil.
- message := messagePayload.message
if err != nil {
+ return apitypes.TypedData{}, err
}
+ message := messagePayload.messageCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| messagePayload, err := createEIP712MessagePayload(data) | |
| message := messagePayload.message | |
| if err != nil { | |
| return apitypes.TypedData{}, err | |
| } | |
| messagePayload, err := createEIP712MessagePayload(data) | |
| if err != nil { | |
| return apitypes.TypedData{}, err | |
| } | |
| message := messagePayload.message |
| // TODO: remove | ||
| if len(digestBz) != crypto.DigestLength { | ||
| digestBz = crypto.Keccak256Hash(digestBz).Bytes() | ||
| } |
There was a problem hiding this comment.
Consider removing the modification of the digest based on its length to ensure consistent behavior.
- // TODO: remove
- if len(digestBz) != crypto.DigestLength {
- digestBz = crypto.Keccak256Hash(digestBz).Bytes()
- }This modification could lead to unexpected behavior and should be avoided unless there's a specific reason for it. Ensure that the input to the Sign function is always the correct length.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // TODO: remove | |
| if len(digestBz) != crypto.DigestLength { | |
| digestBz = crypto.Keccak256Hash(digestBz).Bytes() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- eth/crypto/ethsecp256k1/ethsecp256k1.go (1 hunks)
- eth/crypto/hd/benchmark_test.go (1 hunks)
- eth/ethereum/eip712/eip712_fuzzer_test.go (1 hunks)
- eth/ethereum/eip712/eip712_test.go (1 hunks)
- eth/ethereum/eip712/encoding.go (1 hunks)
- eth/ethereum/eip712/encoding_legacy.go (1 hunks)
- eth/ethereum/eip712/preprocess.go (1 hunks)
- eth/ethereum/eip712/preprocess_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- eth/crypto/ethsecp256k1/ethsecp256k1.go
- eth/crypto/hd/benchmark_test.go
- eth/ethereum/eip712/eip712_fuzzer_test.go
- eth/ethereum/eip712/eip712_test.go
- eth/ethereum/eip712/encoding.go
- eth/ethereum/eip712/encoding_legacy.go
- eth/ethereum/eip712/preprocess.go
- eth/ethereum/eip712/preprocess_test.go
| // legacyDecodeAminoSignDoc attempts to decode the provided sign doc (bytes) as an Amino payload | ||
| // and returns a signable EIP-712 TypedData object. | ||
| func legacyDecodeAminoSignDoc(signDocBytes []byte) (apitypes.TypedData, error) { | ||
| // Ensure codecs have been initialized | ||
| if err := validateCodecInit(); err != nil { | ||
| return apitypes.TypedData{}, err | ||
| } | ||
|
|
||
| var aminoDoc legacytx.StdSignDoc | ||
| if err := aminoCodec.UnmarshalJSON(signDocBytes, &aminoDoc); err != nil { | ||
| return apitypes.TypedData{}, err | ||
| } | ||
|
|
||
| var fees legacytx.StdFee | ||
| if err := aminoCodec.UnmarshalJSON(aminoDoc.Fee, &fees); err != nil { | ||
| return apitypes.TypedData{}, err | ||
| } | ||
|
|
||
| // Validate payload messages | ||
| msgs := make([]sdk.Msg, len(aminoDoc.Msgs)) | ||
| for i, jsonMsg := range aminoDoc.Msgs { | ||
| var m sdk.Msg | ||
| if err := aminoCodec.UnmarshalJSON(jsonMsg, &m); err != nil { | ||
| return apitypes.TypedData{}, fmt.Errorf("failed to unmarshal sign doc message: %w", err) | ||
| } | ||
| msgs[i] = m | ||
| } | ||
|
|
||
| if err := legacyValidatePayloadMessages(msgs); err != nil { | ||
| return apitypes.TypedData{}, err | ||
| } | ||
|
|
||
| // Use first message for fee payer and type inference | ||
| msg := msgs[0] | ||
|
|
||
| // By convention, the fee payer is the first address in the list of signers. | ||
| feePayer := msg.GetSigners()[0] | ||
| feeDelegation := &FeeDelegationOptions{ | ||
| FeePayer: feePayer, | ||
| } | ||
|
|
||
| chainID, err := ethtypes.ParseChainID(aminoDoc.ChainID) | ||
| if err != nil { | ||
| return apitypes.TypedData{}, errors.New("invalid chain ID passed as argument") |
There was a problem hiding this comment.
Consider refining the error message for an invalid chain ID to include more context about the expected format or values.
- return apitypes.TypedData{}, errors.New("invalid chain ID passed as argument")
+ return apitypes.TypedData{}, fmt.Errorf("invalid chain ID '%s' passed as argument", aminoDoc.ChainID)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // legacyDecodeAminoSignDoc attempts to decode the provided sign doc (bytes) as an Amino payload | |
| // and returns a signable EIP-712 TypedData object. | |
| func legacyDecodeAminoSignDoc(signDocBytes []byte) (apitypes.TypedData, error) { | |
| // Ensure codecs have been initialized | |
| if err := validateCodecInit(); err != nil { | |
| return apitypes.TypedData{}, err | |
| } | |
| var aminoDoc legacytx.StdSignDoc | |
| if err := aminoCodec.UnmarshalJSON(signDocBytes, &aminoDoc); err != nil { | |
| return apitypes.TypedData{}, err | |
| } | |
| var fees legacytx.StdFee | |
| if err := aminoCodec.UnmarshalJSON(aminoDoc.Fee, &fees); err != nil { | |
| return apitypes.TypedData{}, err | |
| } | |
| // Validate payload messages | |
| msgs := make([]sdk.Msg, len(aminoDoc.Msgs)) | |
| for i, jsonMsg := range aminoDoc.Msgs { | |
| var m sdk.Msg | |
| if err := aminoCodec.UnmarshalJSON(jsonMsg, &m); err != nil { | |
| return apitypes.TypedData{}, fmt.Errorf("failed to unmarshal sign doc message: %w", err) | |
| } | |
| msgs[i] = m | |
| } | |
| if err := legacyValidatePayloadMessages(msgs); err != nil { | |
| return apitypes.TypedData{}, err | |
| } | |
| // Use first message for fee payer and type inference | |
| msg := msgs[0] | |
| // By convention, the fee payer is the first address in the list of signers. | |
| feePayer := msg.GetSigners()[0] | |
| feeDelegation := &FeeDelegationOptions{ | |
| FeePayer: feePayer, | |
| } | |
| chainID, err := ethtypes.ParseChainID(aminoDoc.ChainID) | |
| if err != nil { | |
| return apitypes.TypedData{}, errors.New("invalid chain ID passed as argument") | |
| return apitypes.TypedData{}, fmt.Errorf("invalid chain ID '%s' passed as argument", aminoDoc.ChainID) |
| // getMsgType returns the message type prefix for the given Cosmos SDK Msg | ||
| func getMsgType(msg sdk.Msg) (string, error) { | ||
| jsonBytes, err := aminoCodec.MarshalJSON(msg) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| var jsonMsg aminoMessage | ||
| if err := json.Unmarshal(jsonBytes, &jsonMsg); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // Verify Type was successfully filled in | ||
| if jsonMsg.Type == "" { | ||
| return "", errors.New("could not decode message: type is missing") |
There was a problem hiding this comment.
Consider refining the error message to include more context about the expected 'Type' field.
- return "", errors.New("could not decode message: type is missing")
+ return "", fmt.Errorf("could not decode message: 'Type' field is missing in JSON: %s", string(jsonBytes))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // getMsgType returns the message type prefix for the given Cosmos SDK Msg | |
| func getMsgType(msg sdk.Msg) (string, error) { | |
| jsonBytes, err := aminoCodec.MarshalJSON(msg) | |
| if err != nil { | |
| return "", err | |
| } | |
| var jsonMsg aminoMessage | |
| if err := json.Unmarshal(jsonBytes, &jsonMsg); err != nil { | |
| return "", err | |
| } | |
| // Verify Type was successfully filled in | |
| if jsonMsg.Type == "" { | |
| return "", errors.New("could not decode message: type is missing") | |
| // getMsgType returns the message type prefix for the given Cosmos SDK Msg | |
| func getMsgType(msg sdk.Msg) (string, error) { | |
| jsonBytes, err := aminoCodec.MarshalJSON(msg) | |
| if err != nil { | |
| return "", err | |
| } | |
| var jsonMsg aminoMessage | |
| if err := json.Unmarshal(jsonBytes, &jsonMsg); err != nil { | |
| return "", err | |
| } | |
| // Verify Type was successfully filled in | |
| if jsonMsg.Type == "" { | |
| return "", fmt.Errorf("could not decode message: 'Type' field is missing in JSON: %s", string(jsonBytes)) |
| - [#1837](https://github.com/NibiruChain/nibiru/pull/1837) - feat(eth): protos, eth types, and evm module types | ||
| - [#1838](https://github.com/NibiruChain/nibiru/pull/1838) - feat(eth): Go-ethereum, crypto, encoding, and unit tests for evm/types | ||
| - [#1841](https://github.com/NibiruChain/nibiru/pull/1841) - feat(eth): Collections encoders for bytes, Ethereum addresses, and Ethereum hashes |
There was a problem hiding this comment.
Ensure consistency in the formatting of changelog entries. Consider adding a period at the end of each entry to maintain uniformity.
- - [#1837](https://github.com/NibiruChain/nibiru/pull/1837) - feat(eth): protos, eth types, and evm module types
- - [#1838](https://github.com/NibiruChain/nibiru/pull/1838) - feat(eth): Go-ethereum, crypto, encoding, and unit tests for evm/types
- - [#1841](https://github.com/NibiruChain/nibiru/pull/1841) - feat(eth): Collections encoders for bytes, Ethereum addresses, and Ethereum hashes
+ - [#1837](https://github.com/NibiruChain/nibiru/pull/1837) - feat(eth): Protos, eth types, and evm module types.
+ - [#1838](https://github.com/NibiruChain/nibiru/pull/1838) - feat(eth): Go-ethereum, crypto, encoding, and unit tests for evm/types.
+ - [#1841](https://github.com/NibiruChain/nibiru/pull/1841) - feat(eth): Collections encoders for bytes, Ethereum addresses, and Ethereum hashes.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| - [#1837](https://github.com/NibiruChain/nibiru/pull/1837) - feat(eth): protos, eth types, and evm module types | |
| - [#1838](https://github.com/NibiruChain/nibiru/pull/1838) - feat(eth): Go-ethereum, crypto, encoding, and unit tests for evm/types | |
| - [#1841](https://github.com/NibiruChain/nibiru/pull/1841) - feat(eth): Collections encoders for bytes, Ethereum addresses, and Ethereum hashes | |
| - [#1837](https://github.com/NibiruChain/nibiru/pull/1837) - feat(eth): Protos, eth types, and evm module types. | |
| - [#1838](https://github.com/NibiruChain/nibiru/pull/1838) - feat(eth): Go-ethereum, crypto, encoding, and unit tests for evm/types. | |
| - [#1841](https://github.com/NibiruChain/nibiru/pull/1841) - feat(eth): Collections encoders for bytes, Ethereum addresses, and Ethereum hashes. |
Purpose / Abstract
Changes
Summary by CodeRabbit
New Features
Documentation
Chores
Refactor